perf(skip masks): skip masks as roaring bitmaps in fetch#421
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
- Coverage 70.65% 70.60% -0.05%
==========================================
Files 219 219
Lines 16967 16998 +31
==========================================
+ Hits 11988 12002 +14
- Misses 4084 4096 +12
- Partials 895 900 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
forshev
force-pushed
the
420-skip-mask-as-roaring-bitmap
branch
from
7d69985 to
ab18105
Compare
| return NewNMergedIterators(iterators), has, nil | ||
| } | ||
|
|
||
| // GetSkipMaskAsRoaringBitmap returns skip masks as roaring bitmap. |
There was a problem hiding this comment.
nit: comment name doesn't match function name
| } | ||
|
|
||
| bitmap := roaring.New() | ||
| mu := &sync.Mutex{} |
There was a problem hiding this comment.
nit: I can't say for sure whether using a mutex hurts performance and by how much, but I'd go with a non-blocking implementation here.
That is, I'd generate a separate bitmap in each goroutine and then merge them at the end.
| } | ||
| } | ||
|
|
||
| if err := l.release(); err != nil { |
There was a problem hiding this comment.
nit: We might exit the function on an error and never call release. In iterators, theoretically, we might also fail to reach release. For me, this is an open question: when is it better to close the file descriptor? Probably, it would be consistent to close the descriptor only when the faction is deleted — just like the index: we keep it open all the time. But that would require a more global refactoring. Alternatively, tie it to the lifetime of skipmaskmanager.loader, but then we would need to return its release() function together with the iterator.
diff --git a/frac/processor/search.go b/frac/processor/search.go
index 3c08eae0..53b2e906 100644
--- a/frac/processor/search.go
+++ b/frac/processor/search.go
@@ -95,7 +95,8 @@ func IndexSearch(
}
m = sw.Start("get_skip_lids")
- skipLIDs, hasSkipLIDs, err := index.GetSkipLIDs(minLID, maxLID, params.Order.IsReverse())
+ skipLIDs, hasSkipLIDs, release, err := index.GetSkipLIDs(minLID, maxLID, params.Order.IsReverse())
+ defer release()
m.Stop()
if err != nil {
return nil, errThe comforting fact is that in Go, when an *os.File is garbage-collected, Close should be called (thanks to runtime.SetFinalizer), but this is not a reliable mechanism.
4 participants
Description
Read skip masks as roaring bitmap for fetch
Fixes #420
If you have used LLM/AI assistance please provide model name and full prompt: